add task solution#2013
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because two explicit task requirements are not yet met and they affect correctness and grading. First, the salary conversion logic (Number(s.replace(/[$,]/g, ''))) is duplicated inline in sortList and getEmployees instead of being encapsulated in the dedicated helper function the task asks for [CHECKLIST ITEM #16]; you should create a small helper (e.g. parseSalary(salaryStr)) and use it everywhere salary strings are turned into numbers. Second, name is currently taken from item.textContent.trim(), which will include all text inside the <li> (name, position, salary, age); the requirement is that name contain only the employee’s name, so you need to select just the appropriate child element or data attribute for the name field, based on the HTML structure [CHECKLIST ITEM #4], [CHECKLIST ITEM #18]. These two issues will cause the tests that check schema and helper usage to fail, so they are considered blocking.
To fix this:
- Extract the repeated salary parsing code into a single helper function and call it from both
sortListandgetEmployees. - Adjust the DOM query for the employee name so that you only capture the name (for example
item.querySelector('.employee-name').textContent.trim()or a similar selector that matches your HTML).
Once you address these points and re-runnpm run test, your overall solution should align well with the remaining requirements, since the sorting logic, numeric conversions, and function structure are otherwise on the right track.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| const items = [...employees.children]; | ||
|
|
||
| items.sort((a, b) => { | ||
| const salaryA = Number(a.getAttribute('data-salary').replace(/[$,]/g, '')); |
There was a problem hiding this comment.
The task explicitly requires a helper function for converting salary from string to number (checklist item #16). Here you perform the conversion inline; consider extracting it into a reusable helper and using that in both sortList and getEmployees.
|
|
||
| items.sort((a, b) => { | ||
| const salaryA = Number(a.getAttribute('data-salary').replace(/[$,]/g, '')); | ||
| const salaryB = Number(b.getAttribute('data-salary').replace(/[$,]/g, '')); |
There was a problem hiding this comment.
Same salary parsing logic is duplicated here; it should be delegated to the helper function required by the task instead of repeating the regex/Number conversion.
|
|
||
| const getEmployees = (employees) => { | ||
| return [...employees.children].map((item) => ({ | ||
| name: item.textContent.trim(), |
There was a problem hiding this comment.
Using item.textContent.trim() for name will likely include the entire text of the list item (e.g. name + position + salary + age). The schema expects only the employee name, so you should extract just the name field based on the HTML structure (e.g. a specific child element or data attribute).
| return [...employees.children].map((item) => ({ | ||
| name: item.textContent.trim(), | ||
| position: item.getAttribute('data-position'), | ||
| salary: Number(item.getAttribute('data-salary').replace(/[$,]/g, '')), |
There was a problem hiding this comment.
Salary parsing logic is repeated again here; per the description you should write and use a dedicated helper function to convert salary strings to numbers instead of duplicating this code.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it meets the core functional requirements and aligns with the task description. The sortList(list) function correctly takes the <ul> element, converts data-salary strings to numbers via the shared parseSalary helper, sorts employees in descending order by salary, and re-appends the sorted <li> elements to the list. The getEmployees(list) function properly maps each list item into an object with the exact schema { name, position, salary, age }, using data-name (not the full text content) for the name field and using the same salary parsing logic for consistency. Both functions are invoked at the bottom of main.js, and the logic is contained only within this file as required. I don’t see any critical or blocking issues that would prevent the tests or the specified behavior from working as intended. Nice work consolidating the salary parsing and making the data extraction clean and focused on the required attributes.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.